Fix bug 54254 - #128

Not sure about this patch, since month names are not according to wiki anyway - 1st month should be Nisan, and Adar/AdarII should be 12th, also in non-leap years it should be just "Adar". Though you are right that second Adar is the same Adar as in non-leap years. But I think it'd be better if it were called by right name.

For the numbering, see
http://en.wikipedia.org/wiki/Hebrew_calendar#Names_of_months - for
almost all purposes, Tishrei is the first month of the year.
"The day most commonly referred to as the "New Year" is 1 Tishrei,
which actually begins in the seventh month of the ecclesiastical year.
On that day the formal New Year for the counting of years (such as
Shmita and Yovel), Rosh Hashanah ("head of the year") is observed.
(see Ezekiel 40:1, which uses the phrase "beginning of the year".)
This is the civil new year, and the date on which the year number
advances." etc.

As for the names, I agree that ideally the month should be called Adar
in non-leap years, but my C isn't very good and it seemed far from
trivial to make that change. This patch addresses two issues, though:
first, that the month in a leap year should be Adar II instead of Adar
I (this is relevant because the holiday of Purim occurs each year in
Adar, and Adar II in leap years, so the clear expectation is that Adar
I should be the one that only exists in leap years. The second,
related issue is that in a non-leap year, the unpatched functions will
return 6, or Adar I for the month, while cal_days_in_month(CAL_JEWISH,
6, 5772) = 0, implying that the current month has zero days (this is
because cal_days_in_month correctly believes that Adar II exists in a
non-leap year and Adar I does not). After this patch, jdtojewish and
cal_days_in_month are consistent, so the current month is never one
with zero days (since both agree that Adar I occurs only in leap
years, which, again, is the common expectation of people familiar with
the Jewish calendar).

On Sat, Jul 14, 2012 at 9:50 PM, Stanislav Malyshev
reply@reply.github.com
wrote:

Not sure about this patch, since month names are not according to wiki anyway - 1st month should be Nisan, and Adar/AdarII should be 12th, also in non-leap years it should be just "Adar". Though you are right that second Adar is the same Adar as in non-leap years. But I think it'd be better if it were called by right name.

---

Reply to this email directly or view it on GitHub:
#128 (comment)

I see what you mean. I'll try to see if I can fix the Adar issue. Could you also add test for the cal_days_in_month() issue and its interaction with jdtojewish()?

I'm not sure that additional tests are necessary for cal_days_in_month

• its behavior has actually been correct all along, it's just that the
  bug with jdtojewish was magnified when its output was used as input
  for cal_days_in_month (because cal_days_in_month was aware that Adar I
  doesn't exist in non-leap years, but jdtojewish and other functions
  calling the same C function were giving Adar I as the month). I did
  add another commit bringing the documentation in jewish.c in line with
  the updated code.

On Sat, Jul 14, 2012 at 11:13 PM, Stanislav Malyshev
reply@reply.github.com
wrote:

I see what you mean. I'll try to see if I can fix the Adar issue. Could you also add test for the cal_days_in_month() issue and its interaction with jdtojewish()?

---

Reply to this email directly or view it on GitHub:
#128 (comment)

Ping

I was just thinking about the test showing interaction between cal_days_in_month and jdtojewish to see that it returns correct result in every combination of cases.

So maybe a test that prints the number of days in every month for a two year span including both a leap and a non-leap year and checks that they're all correct?

sounds good

Test added. Are you still planning to fix the Adar name issue? It seems the only way is to explicitly check anywhere that month names are returned (in English or Hebrew) whether month=7 and it's a leap year - it could be done by having Adar and Adar II have different month numbers, but that would break BC and generally be a poor solution. I'm not sure whether the extra name can safely be added to the arrays that hold all the names currently without side effects, but it could just be stored in its own variable since array indexing won't add anything here.

Please check out this pull: #149
It integrates your fixes and improves display of Adar month.

Comment on behalf of stas at php.net:

merged